Code Review — PR #361

Review Information

• PR: [#226] feat: pair-capability-assess-cost — multi-provider cost class at review #361 — [#226] feat: pair-capability-assess-cost — multi-provider cost class at review
• Author: rucka
• Reviewer: Independent review (/pair-process-review)
• Review Date: 2026-07-19
• Story: pair-capability-assess-cost — multi-provider cost class at review #226 — pair-capability-assess-cost
• Review Type: Feature (KB skill + guideline + conformance tests + doc/count cascade)

Review Summary

Overall Assessment

• Approved with Comments — Minor issues noted; one Major item is a disclosed, tracked scope-deferral (AC3 → Extended code-review template (security + cost, reading budget) #228) that needs a human merge-gate decision.

Key Changes Summary

Adds the assess-cost capability skill (output-only chromatic cost class green/yellow/orange/red), its KB guideline (multi-provider cost-signal catalog + general/AWS heuristics + gotchas), a 27-test structural conformance guard, and a thorough skill-count cascade (38→39 / 28→29) across KB prose, .claude/skills + root mirrors, and apps/website docs. Wired into quality-model §3.3/§7 and the QA README.

Business Value

Completes the Cost dimension of the classification matrix (Epic #208), parallel to the shipped assess-security. Realizes the forward references ADR-013/§3.3 left for #226.

Functionality Review — AC verification

Detailed Review Comments

Major

• [story pair-capability-assess-cost — multi-provider cost class at review #226 / PR body — AC3 + "red ⇒ blocking sign-off" addendum] The review-side composition (cost section rendered at review, AC3) and the story addendum's "red class ⇒ blocking human sign-off" are not delivered in this PR. They are deferred to Extended code-review template (security + cost, reading budget) #228 (extended code-review template + review-output alignment), which is Refined, explicitly owns the review-template/review-output wiring, and lists pair-capability-assess-cost — multi-provider cost class at review #226 as a prerequisite ("sections feed from assess-cost pair-capability-assess-cost — multi-provider cost class at review #226"). Deferral is architecturally correct: an output-only classifier must never block, and the verdict-area/blocking behavior is a /review responsibility Extended code-review template (security + cost, reading budget) #228 wires. However, the PR body states "Closes pair-capability-assess-cost — multi-provider cost class at review #226", which would auto-close the story with AC3 (and the DoD item "Composition with review verified") unmet. Recommendation (human merge-gate decision): either drop the "Closes pair-capability-assess-cost — multi-provider cost class at review #226" auto-close and keep pair-capability-assess-cost — multi-provider cost class at review #226 open until Extended code-review template (security + cost, reading budget) #228 lands, or explicitly move AC3 + the "red⇒sign-off" addendum to Extended code-review template (security + cost, reading budget) #228 in the story text before merge. No code change in this PR. Disposition: Deferred to Extended code-review template (security + cost, reading budget) #228.

Minor

• [quality-model.md §2, line 21 (dataset + root mirror)] The Pillars table still reads `assess-cost` (cost-signal catalog, forthcoming). This PR de-staled §3.3 and §7 (removed "forthcoming"/"not yet published") but missed §2. Now that the skill and catalog ship in this PR, "forthcoming" is inaccurate. Recommendation: drop "forthcoming" on line 21 in both the dataset and root-mirror copies (e.g. `assess-cost` (cost-signal catalog)).

• [assess-cost/SKILL.md line 105 / §3.2 grounding] The skill cites "§3.2 floor semantics applied to the cost class" (review may only confirm-or-raise, never lower). But §3.2's floor/never-lower rule is phrased about the risk tier (max), and §3.2 explicitly states cost is not part of that max ("it is its own class, computed independently"). §3.3 does not itself state a floor rule for the cost class. The extension from the D17 "built twice" principle is reasonable, but the citation is a mild extrapolation. Recommendation (low priority): either add an explicit floor sentence to §3.3 for the cost class, or soften the skill's "§3.2 ... applied to the cost class" wording to reference D17 "built twice" instead.

• [packages/knowledge-hub/src/conformance/assess-cost.test.ts] The conformance test is structural-only (string-contains over the markdown) — consistent with the sibling assess-security.test.ts convention for prompt-artifact skills, so acceptable. But the story's testing strategy called for behavioral calibration ("4 sample changes, one per class, + one unknown-surface change") and the DoD lists "standalone invocation verified" — manual-QA activities whose completion is not evidenced in the PR. Recommendation: confirm the sample-diff calibration was performed (the story's own risk note — "heuristics too generic → everything medium/orange" — depends on it), or track it as follow-up QA.

Questions

• [guideline / story edge-case wording] The story's Edge Cases say unknown surface ⇒ "medium"; the implementation uses orange. This reads as consistent with the 4-level chromatic scale (green<yellow<orange<red, medium↔orange, the more-conservative of the two middle classes) and the "conservative, visible, never silently green" intent — please confirm orange (not yellow) is the intended mapping.

Technical Standards / Code Quality

• Skill structure mirrors assess-security faithfully (frontmatter, Arguments, Rule Set cascade, Algorithm steps, Output Format, Composition Interface, Edge Cases, Graceful Degradation, Notes). D17/D21 layering respected — no heuristics or provider names live in the skill; the catalog + heuristics live in the guideline; R2.13 provider-agnostic core is real.
• Mirror self-consistency (PR fix: make pair update self-consistent for the dogfooded root install #346 invariant) holds: /assess-cost slash-refs correctly rewritten to /pair-capability-assess-cost in the .claude/skills mirror; KB doc links resolve identically (dataset-skill and mirror-skill are both 3 levels above their respective .pair/knowledge); check:links = 0 patched.
• Section references (§2, §3.2, §3.3, §5, §7) all verified accurate against quality-model.md; guideline internal links (cost-optimization / aws-deployment / provider-evaluation / multi-cloud) all resolve.
• Skill-count cascade is complete and internally consistent: no stale 38/28 capability remains anywhere in KB, mirrors, or website; catalog-derivation math (9 process + 29 capability + /next = 39) updated in next/SKILL.md; assess-output-only.test.ts correctly leaves assess-cost out of its "8 decision assess-*" list (assess-cost is a review-classifier with its own dedicated test, like assess-security).
• All 27 conformance assertions verified by inspection against the actual files — they resolve.

Testing Review

• 27 new structural conformance tests (dataset + mirror parity for skill, guideline, catalog, counts). Test-first (RED→GREEN) claimed. Coverage is structural, not behavioral — acceptable by the established markdown-skill convention (see Minor above re: manual calibration).

Security Review

• green — no findings. Documentation/KB-only change (markdown skill + guideline + test + doc counts). No executable product surface, no inputs, no secrets, no dependencies introduced. assess-cost is output-only and writes nothing.

Adoption Compliance

• No new technical decision introduced — implements decisions already recorded (ADR-013, D14/D17/D21/D22, R2.13/R6.1/R6.2). No new ADR/ADL required (correctly none added). Conforms to the assess-* family conventions and the quality-model three-layer principle.

Tech Debt

• None introduced. The two doc-precision Minors (§2 "forthcoming"; §3.3 cost-floor grounding) are trivial in-scope cleanups, not debt.

Decision

Approved with Comments. The code that is present is correct, complete for its stated scope (AC1/AC2/AC4), consistent across every mirror/count/doc surface, and cleanly follows the sibling convention. The one Major item (AC3 + red-sign-off deferred to #228) is disclosed and tracked; the only blocking-adjacent question is process, not code: the human merge gate should resolve the "Closes #226"-with-AC3-unmet tension (keep #226 open until #228, or move AC3 to #228). Recommended in-PR fix before merge: the §2 "forthcoming" stale line (Minor).

Remediation — round 1 (commit d9627e7)

Per finding:

• [Minor] quality-model.md:21 cites assess-cost as "(cost-signal catalog, forthcoming)" (§2 Pillars table) → dropped "forthcoming" (the skill + catalog ship in this PR); applied in BOTH copies — .pair/knowledge/guidelines/quality-assurance/quality-model.md, packages/knowledge-hub/dataset/.pair/knowledge/guidelines/quality-assurance/quality-model.md
• [Minor] SKILL.md:105 cites «§3.2 floor semantics applied to the cost class» (§3.2 is the risk-tier max, computes cost independently) → softened to cite D17 "built twice": the class is estimated from story context at refinement and re-derived from the diff at review, the more conservative of the two wins; applied in BOTH copies — packages/knowledge-hub/dataset/.skills/capability/assess-cost/SKILL.md, .claude/skills/pair-capability-assess-cost/SKILL.md
• [Questions/vocab] scale vocabulary green/low/medium/red vs green/yellow/orange/red → the files I own already use the 4-level chromatic scale cost:green|yellow|orange|red consistently (guideline cost-assessment.md §3.3 + assess-cost SKILL), and unknown-surface → orange is already the documented conservative default. The low/medium variant appears only in the issue body, which was intentionally not touched. No change needed in guideline/skill.
• [Minor · QA] calibration (4 sample-diff + unknown-surface) and "standalone invocation verified" → not automated (would be fictitious tests). Tracked as a manual-QA follow-up (see below).

Not changed (escalated): none.

Follow-up QA (manual, tracked): calibrate assess-cost against 4 representative sample diffs + an unknown-surface case, and verify standalone /assess-cost invocation end-to-end. To be run at #228 (review-side wiring) alongside AC3/rendering.

Note: PR body updated Closes #226 → Refs #226 — #226 stays open until #228 delivers AC3/rendering.

Quality gates: PASS — assess-cost conformance (27) + transfer-dataset root-equality (29) + skills-guide-mirror (15) tests green; check:links clean (dataset + root); mdlint clean; ts:check green (pre-commit).
→ Re-review requested.